Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add catalog UI to browse the catalog #68

Closed
wants to merge 1 commit into from

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Oct 21, 2024

related to podman-desktop/podman-desktop#8972

Screen.Recording.2024-10-21.at.11.33.25.mov

Change-Id: I820ba559ae561308bca38dc867439615d6e8d8af

@axel7083
Copy link
Contributor

I am not sure to follow, is this going to be iframed in the website ?

@benoitf
Copy link
Contributor Author

benoitf commented Oct 21, 2024

I am not sure to follow, is this going to be iframed in the website ?

yes, to reuse most of UI svelte components

@benoitf
Copy link
Contributor Author

benoitf commented Oct 21, 2024

how it renders within the website:

Screen.Recording.2024-10-21.at.11.45.56.mov

Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using static/api folder structure ? static do not really provide apis... ?

@benoitf
Copy link
Contributor Author

benoitf commented Oct 22, 2024

Why using static/api folder structure ? static do not really provide apis... ?

it's to have the current content of the website https://github.com/containers/podman-desktop-catalog/tree/gh-pages/api at the proper location

Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as design is something we could iterate later

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remarks:

  • UI is quite rude, maybe worth adding a header ?
  • Seems to be some components are duplicated, maybe follow up PR to get rid of it
  • this PR holds a copy of the catalog so is it temporary
  • When we list extension versions maybe worth having a link to install that specific version
  • Validating workflow missing, follow up PR ?

.gitignore Outdated
/.svelte-kit
/.eslintcache
/coverage
/package
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

src/app.html Outdated
<link rel="icon" href="%sveltekit.assets%/favicon.png" />
<meta name="viewport" content="width=device-width" />
%sveltekit.head%
</head>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding a title ?

}

test('check initial light theme', async () => {
const { baseElement } = render(Appearance, {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { baseElement } = render(Appearance, {});
const { baseElement } = render(Appearance);

}
</script>

<button
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Button componenrt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so here it's using button to say that we have an active element with a onclick trigger (a card) but it doesn't look at all as a button

import { catalogExtensions } from '$lib/extensions.svelte';

import catalogOfExtensions from '../../static/api/extensions.json';
import Page from '././+page.svelte';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import Page from '././+page.svelte';
import Page from './+page.svelte';

@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

UI is quite rude, maybe worth adding a header ?

You'll will see it through podman desktop website
cf #68 (comment)

@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

Seems to be some components are duplicated, maybe follow up PR to get rid of it

what are the components duplicated ?

@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

this PR holds a copy of the catalog so is it temporary

it's not, it's how the files will be served (you won't need to create a PR to the gh-pages branch, it will be against this folder)

@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

When we list extension versions maybe worth having a link to install that specific version

you don't have the ability to install a specific version (no link possible for the moment)

@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

Validating workflow missing, follow up PR ?

yes here I was waiting on how to proceed to merge such a big PR, I will split with chunks

@slemeur
Copy link

slemeur commented Oct 24, 2024

This is looking great as a first step, I think we can move forward with this and create follow-up issues for improving the experience and the UI accordingly with the mockups on the points mentioned before.

@benoitf benoitf marked this pull request as draft October 24, 2024 08:02
@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

first batch: #69 (setup of the project)

I've included GH action

@benoitf benoitf force-pushed the DESKTOP-8972 branch 2 times, most recently from fcbf16d to 898f9a4 Compare October 24, 2024 14:39
related to podman-desktop/podman-desktop#8972

Signed-off-by: Florent Benoit <[email protected]>
Change-Id: I9e121490ccf65275ec1273f716d9d7a8e26d791a
@benoitf
Copy link
Contributor Author

benoitf commented Oct 24, 2024

replaced by #72

@benoitf benoitf closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants